Use correct to_remote script in counterparty commitments #2605

wpaulino · 2023-09-26T23:48:37Z

While our commitment transactions did use the correct to_remote script, the ChannelMonitor's was not as it is tracked separately. This would lead to users never receiving an Event::SpendableOutputs with a StaticPaymentOutput descriptor to claim the funds.

Luckily, any users affected which had channel closures confirmed by a counterparty commitment just need to replay the closing transaction to receive the event.

lightning/src/sign/mod.rs

codecov-commenter · 2023-09-27T23:25:32Z

Codecov Report

Attention: 23 lines in your changes are missing coverage. Please review.

Comparison is base (6016101) 89.03% compared to head (f464aa9) 88.99%.
Report is 20 commits behind head on main.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2605      +/-   ##
==========================================
- Coverage   89.03%   88.99%   -0.04%     
==========================================
  Files         112      112              
  Lines       86351    86706     +355     
  Branches    86351    86706     +355     
==========================================
+ Hits        76879    77167     +288     
- Misses       7235     7308      +73     
+ Partials     2237     2231       -6

Files	Coverage Δ
lightning/src/ln/monitor_tests.rs	`98.30% <100.00%> (-0.04%)`	⬇️
lightning/src/util/test_utils.rs	`77.06% <100.00%> (+0.10%)`	⬆️
lightning/src/events/bump_transaction.rs	`82.33% <0.00%> (ø)`
lightning/src/ln/chan_utils.rs	`92.09% <62.50%> (+0.02%)`	⬆️
lightning/src/chain/channelmonitor.rs	`84.86% <73.91%> (-0.02%)`	⬇️
lightning/src/sign/mod.rs	`74.05% <73.91%> (-0.36%)`	⬇️

... and 11 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

lightning/src/sign/mod.rs

lightning/src/chain/channelmonitor.rs

lightning/src/sign/mod.rs

TheBlueMatt · 2023-09-28T19:05:41Z

The commit titled Fix off-by-one max witness estimate for P2WPKH StaticPaymentDescriptor isn't fixing anything - its replacing 1 + 73 + 34 with 1 + 73 + 1 + 33, which are the same :). Its a nice cleanup to use a constant, though.

wpaulino · 2023-09-28T19:11:30Z

The commit titled Fix off-by-one max witness estimate for P2WPKH StaticPaymentDescriptor isn't fixing anything - its replacing 1 + 73 + 34 with 1 + 73 + 1 + 33, which are the same :). Its a nice cleanup to use a constant, though.

It is, check again :) We previously had an estimate of 108, now 109.

TheBlueMatt · 2023-09-28T20:29:19Z

Oh lol the extra one I skimmed past.

lightning/src/ln/monitor_tests.rs

lightning/src/sign/mod.rs

jkczyz · 2023-09-28T21:45:32Z

lightning/src/sign/mod.rs

-		let remotepubkey = self.pubkeys().payment_point;
-		let witness_script = bitcoin::Address::p2pkh(&::bitcoin::PublicKey{compressed: true, inner: remotepubkey}, Network::Testnet).script_pubkey();
+		let remotepubkey = bitcoin::PublicKey::new(self.pubkeys().payment_point);
+		let witness_script = if self.channel_type_features().supports_anchors_zero_fee_htlc_tx() {
+			chan_utils::get_to_countersignatory_with_anchors_redeemscript(&remotepubkey.inner)
+		} else {
+			Script::new_p2pkh(&remotepubkey.pubkey_hash())
+		};
 		let sighash = hash_to_message!(&sighash::SighashCache::new(spend_tx).segwit_signature_hash(input_idx, &witness_script, descriptor.output.value, EcdsaSighashType::All).unwrap()[..]);
 		let remotesig = sign_with_aux_rand(secp_ctx, &sighash, &self.payment_key, &self);
-		let payment_script = bitcoin::Address::p2wpkh(&::bitcoin::PublicKey{compressed: true, inner: remotepubkey}, Network::Bitcoin).unwrap().script_pubkey();
+		let payment_script = if self.channel_type_features().supports_anchors_zero_fee_htlc_tx() {
+			witness_script.to_v0_p2wsh()
+		} else {
+			Script::new_v0_p2wpkh(&remotepubkey.wpubkey_hash().unwrap())
+		};

 		if payment_script != descriptor.output.script_pubkey { return Err(()); }

 		let mut witness = Vec::with_capacity(2);
 		witness.push(remotesig.serialize_der().to_vec());
 		witness[0].push(EcdsaSighashType::All as u8);
-		witness.push(remotepubkey.serialize().to_vec());
+		if self.channel_type_features().supports_anchors_zero_fee_htlc_tx() {
+			witness.push(witness_script.to_bytes());
+		} else {
+			witness.push(remotepubkey.to_bytes());
+		}


We condition on this quite a bit. Wonder if it would be worth type parameterizing to avoid missing places in the future. Doesn't need to be done now, of course.

Mind giving a more concrete suggestion?

This will change even further once we have taproot and we need different sighash and signing methods.

I was thinking that rather than checking against features, you initialized the signer with a parameterization for the channel type. Then instead of having an increasingly complex condition for each of these three assignments, each has it's own implementation. The strategy pattern, essentially. Though, in practice this parameterization may need to trickle up to ChannelMonitor and possibly further, so might not be worth it.

Alternatively, could just use an enum as mentioned later in another comment.

I was thinking that rather than checking against features, you initialized the signer with a parameterization for the channel type. Then instead of having an increasingly complex condition for each of these three assignments, each has it's own implementation. The strategy pattern, essentially. Though, in practice this parameterization may need to trickle up to ChannelMonitor and possibly further, so might not be worth it.

This would be quite a big change if we intend to apply it to all other signing methods on InMemorySigner, since it wouldn't make sense to have it on some but not others.

I like the idea though, especially as we add taproot and further channel/commitment upgrades.

jkczyz · 2023-09-28T22:00:08Z

lightning/src/sign/mod.rs

+					let sequence =
+						if descriptor.channel_transaction_parameters.as_ref()
+							.map(|channel_params| channel_params.channel_type_features.supports_anchors_zero_fee_htlc_tx())
+							.unwrap_or(false)
+						{
+							Sequence::from_consensus(1)
+						} else {
+							Sequence::ZERO
+						};
 					input.push(TxIn {
 						previous_output: descriptor.outpoint.into_bitcoin_outpoint(),
 						script_sig: Script::new(),
-						sequence: Sequence::ZERO,
+						sequence,
 						witness: Witness::new(),
 					});
-					witness_weight += StaticPaymentOutputDescriptor::MAX_WITNESS_LENGTH;
+					witness_weight += descriptor.max_witness_length();


Hmmm... I'd definitely lean towards a separate variant, FWIW. Seems cleaner.

The thought of adding a new variant came from a confusion between StaticOutput and StaticPaymentOutput descriptors. Once we upgrade to Taproot, we'd also need a new variant because we're going from P2WSH to P2TR. Not sure it's worth having all these different types when it's still the same output (to_remote on a counterparty commitment) we're claiming backed by the same key.

I see. My thinking was that by explicitly enumerating the cases, we would be forced to consider all of them and avoid bugs. I suppose an alternative would be to have a mapping from ChannelTypeFeatures to an enum with a variant for each type that we could match on.

I'll see if I can come up with something tomorrow, it'll have to wait for when taproot is added here otherwise. In any case, the best way to avoid bugs here would be test coverage via check_spends!() 😛

SGTM. A mapping wouldn't require any serialization changes, too.

Just to expand on this based on offline discussion. We'll keep the variants as they are in this PR. This will give us a ChannelTransactionParameters which has a ChannelTypeFeatures. Later, we can add something like a to_channel_type method to ChannelTypeFeatures for internal use that will give an enum for us to match so as to avoid missing cases.

Using either another variant or a nested enum instead could be error prone as it may go out of sync with ChannelTypeFeatures.

lightning/src/sign/mod.rs

ariard

Code sounds good.

ariard · 2023-09-29T01:28:54Z

lightning/src/ln/monitor_tests.rs

 		if let Event::SpendableOutputs { outputs, channel_id } = event {
 			assert_eq!(outputs.len(), 1);
 			assert!(vec![chan_b.2, chan_a.2].contains(&channel_id.unwrap()));
 			let spend_tx = nodes[0].keys_manager.backing.spend_spendable_outputs(
 				&[&outputs[0]], Vec::new(), Script::new_op_return(&[]), 253, None, &Secp256k1::new(),
 			).unwrap();

-			check_spends!(spend_tx, revoked_claim_transactions.get(&spend_tx.input[0].previous_output.txid).unwrap());
+			if let SpendableOutputDescriptor::StaticPaymentOutput(_) = &outputs[0] {


i think one way to harden the test can be to count the StaticPaymentOutput with num_static_payment_output and check at the end the right number of this type of descriptor has been found.

lightning/src/sign/mod.rs

lightning/src/chain/channelmonitor.rs

While our commitment transactions did use the correct `to_remote` script, the `ChannelMonitor`'s was not as it is tracked separately. This would lead to users never receiving an `Event::SpendableOutputs` with a `StaticPaymentOutput` descriptor to claim the funds. Luckily, any users affected which had channel closures confirmed by a counterparty commitment just need to replay the closing transaction to receive the event.

`to_remote` outputs on commitment transactions with anchor outputs have an additional `1 CSV` constraint on its spending condition, transitioning away from the previous P2WPKH script to a P2WSH. Since our `ChannelMonitor` was never updated to track the proper `to_remote` script on anchor outputs channels, we also missed updating our signer to handle the new script changes.

We were not accounting for the extra byte denoting the number of items in the witness stack.

wpaulino · 2023-09-29T20:48:53Z

Had to rebase to use get_spendable_outputs in the new test so I used fixups this time around to better show what changed.

jkczyz · 2023-09-29T21:16:44Z

Thanks for adding the test! LGTM. Fee free to squash the fixups.

wpaulino added this to the 0.0.117 milestone Sep 26, 2023

TheBlueMatt reviewed Sep 27, 2023

View reviewed changes

lightning/src/sign/mod.rs Show resolved Hide resolved

wpaulino mentioned this pull request Sep 27, 2023

Fix matching of second-stage HTLC claim in get_htlc_balance #2610

Merged

wpaulino force-pushed the anchors-monitor-track-to-remote-script branch from 37f3469 to 3c1b187 Compare September 27, 2023 23:24

ariard reviewed Sep 28, 2023

View reviewed changes

lightning/src/sign/mod.rs Outdated Show resolved Hide resolved

wpaulino force-pushed the anchors-monitor-track-to-remote-script branch 3 times, most recently from f6b21c9 to 1fb2a41 Compare September 28, 2023 16:42

TheBlueMatt reviewed Sep 28, 2023

View reviewed changes

lightning/src/chain/channelmonitor.rs Show resolved Hide resolved

TheBlueMatt reviewed Sep 28, 2023

View reviewed changes

lightning/src/sign/mod.rs Show resolved Hide resolved

wpaulino force-pushed the anchors-monitor-track-to-remote-script branch from 1fb2a41 to 9113ea3 Compare September 28, 2023 19:33

jkczyz reviewed Sep 28, 2023

View reviewed changes

wpaulino force-pushed the anchors-monitor-track-to-remote-script branch from 9113ea3 to 163ad99 Compare September 28, 2023 22:47

ariard reviewed Sep 29, 2023

View reviewed changes

wpaulino mentioned this pull request Sep 29, 2023

Consider mapping ChannelTypeFeatures to concrete typed enum to ease integration of new channel types #2627

Open

jkczyz previously approved these changes Sep 29, 2023

View reviewed changes

lightning/src/chain/channelmonitor.rs Outdated Show resolved Hide resolved

TheBlueMatt reviewed Sep 29, 2023

View reviewed changes

lightning/src/chain/channelmonitor.rs Outdated Show resolved Hide resolved

wpaulino added 3 commits September 29, 2023 13:46

Fix off-by-one max witness estimate for P2WPKH StaticPaymentDescriptor

3299d88

We were not accounting for the extra byte denoting the number of items in the witness stack.

wpaulino dismissed jkczyz’s stale review via 5607977 September 29, 2023 20:47

wpaulino force-pushed the anchors-monitor-track-to-remote-script branch from 163ad99 to 5607977 Compare September 29, 2023 20:47

wpaulino added 2 commits September 29, 2023 14:22

Fix incorrect anchors counterparty_payment_script upon deserialization

9f3bb7d

Expose witness_script for StaticPaymentOutputDescriptor

f464aa9

wpaulino force-pushed the anchors-monitor-track-to-remote-script branch from 5607977 to f464aa9 Compare September 29, 2023 21:22

TheBlueMatt approved these changes Sep 29, 2023

View reviewed changes

jkczyz approved these changes Sep 29, 2023

View reviewed changes

TheBlueMatt merged commit 620244d into lightningdevkit:main Sep 29, 2023

wpaulino deleted the anchors-monitor-track-to-remote-script branch September 29, 2023 22:07

jkczyz mentioned this pull request Oct 3, 2023

Avoid unwrap'ing channel_parameters in to_counterparty signing #2634

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use correct to_remote script in counterparty commitments #2605

Use correct to_remote script in counterparty commitments #2605

wpaulino commented Sep 26, 2023

codecov-commenter commented Sep 27, 2023 •

edited

Loading

TheBlueMatt commented Sep 28, 2023

wpaulino commented Sep 28, 2023

TheBlueMatt commented Sep 28, 2023

jkczyz Sep 28, 2023

wpaulino Sep 28, 2023

wpaulino Sep 28, 2023

jkczyz Sep 29, 2023

wpaulino Sep 29, 2023

wpaulino Sep 29, 2023

jkczyz Sep 28, 2023

wpaulino Sep 28, 2023 •

edited

Loading

jkczyz Sep 29, 2023

wpaulino Sep 29, 2023

jkczyz Sep 29, 2023

jkczyz Sep 29, 2023

ariard left a comment

ariard Sep 29, 2023

wpaulino commented Sep 29, 2023 •

edited

Loading

jkczyz commented Sep 29, 2023

Use correct to_remote script in counterparty commitments #2605

Use correct to_remote script in counterparty commitments #2605

Conversation

wpaulino commented Sep 26, 2023

codecov-commenter commented Sep 27, 2023 • edited Loading

Codecov Report

TheBlueMatt commented Sep 28, 2023

wpaulino commented Sep 28, 2023

TheBlueMatt commented Sep 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wpaulino Sep 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ariard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wpaulino commented Sep 29, 2023 • edited Loading

jkczyz commented Sep 29, 2023

codecov-commenter commented Sep 27, 2023 •

edited

Loading

wpaulino Sep 28, 2023 •

edited

Loading

wpaulino commented Sep 29, 2023 •

edited

Loading